-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
B 21170 INT 2 - Ensure USPostRegionsCitiesID populates in a scenarios #14467
Conversation
Bundle StatsHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded
Removed
Bigger
Smaller No assets were smaller Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accepted Happo diffs, reviewing further now
@@ -67,7 +67,7 @@ type DeleteMTOShipmentHandler struct { | |||
services.ShipmentDeleter | |||
} | |||
|
|||
// Handle handler that updates a mto shipment | |||
// Handle handler that deletes a mto shipment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MVP for fixing stuff like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to this task: Is it strange that we don't show tertiary addresses on the move details page?
Back on topic - I'm curious, there's two postal code fields when making an Origin SIT service item. This is intentional, correct?:
Finally, all test steps completed successfully. However, I'm not seeing the us_post_region_cities_id value being populated in the addresses table. I do see it in the city finder's network log. There's also one instance of the string of 0's. This is after checking out the branch, making a fresh db, and running through test steps with 33764, 40509, and 40517, which should all be valid zips. Going to do some digging and see why those 0's appeared, but please let me know what you think of this behavior.
Looks like the all 0's came from test steps 10 and 11. |
Failed Passed |
@@ -301,6 +302,12 @@ func Address(address *models.Address) *primemessages.Address { | |||
County: address.County, | |||
ETag: etag.GenerateEtag(address.UpdatedAt), | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw this too, @ajlusk just fixed it so do a pull of latest and test it again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved in commits right before you posted this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pulled latest and I still see the issue.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything has an id for me, I didn't see anymore null or any id with all 0's.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After pulling latest, I no longer have any of the aforementioned issues. Still curious about the double sit zips and the tertiary addresses not showing, but those aren't really the focus of this work, so we'll discuss them another time. Well done!
Both fair concerns but both outside the scope of this. |
B-21170
Summary
Similar to an issue found in 21169's INT PR the new USPostRegionsCitiesID wasn't populating correctly in all scenarios. Often the DB contained all zeros (a.k.a. uuid.Nil). This should resolve that.
Verification Steps for the Author
These are to be checked by the author.
Verification Steps for Reviewers
These are to be checked by a reviewer.
Setup to Run the Code
How to test
We have 6 forms to verify use the city finder and populate the USPostRegionsCitiesID.
Frontend
officeApp
class or custommin-width
styling is used to hide any states the would not be visible to the user.Backend
Database
Any new migrations/schema changes:
Screenshots